-
-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add type annotations to the project and run mypy on CI #11
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
@@ -40,6 +40,7 @@ project_urls = | |||
[options] | |||
packages = find: | |||
python_requires = >=3.7 | |||
include_package_data = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without this line, the py.typed
file will not be installed to the Python site-packages
directory.
Observe:
$ cd termcolor
$ pip install .
$ ls ~/.virtualenvs/termcolor/lib/python3.9/site-packages/termcolor/
... should include py.typed
Adjust the ls
to be your virtualenv.
The py.typed
file is necessary for PEP-561 compliance. More detail at:
https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-library-stubs-or-py-typed-marker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff, thanks!
src/termcolor/termcolor.py
Outdated
def colored(text, color=None, on_color=None, attrs=None): | ||
def colored( | ||
text: str, | ||
color: Optional[str] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use from __future__ import annotations
and do things like this?
color: Optional[str] = None, | |
color: str | None = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea! Applied this suggestion in the latest revision.
This helps library consumers ensure that they are using the provided API correctly. The project includes a py.typed file for PEP-561 compliance. This also helps ensure internal correctness and consistency. mypy also runs on the tests, to demonstrate the test cases are representative and real use cases. Type checking will run on all pull requests through GitHub actions and pre-commit.
Codecov Report
@@ Coverage Diff @@
## main #11 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 3 3
Lines 65 70 +5
=========================================
+ Hits 65 70 +5
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Thank you! |
This helps library consumers ensure that they are using the provided API
correctly. The project includes a py.typed file for PEP-561 compliance.
This also helps ensure internal correctness and consistency. mypy also
runs on the tests, to demonstrate the test cases are representative and
real use cases.
Type checking will run on all pull requests through GitHub actions and
pre-commit.